Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

sys/net/nanocoap: fix buffer overflow in separate response handling #21075

Merged
merged 5 commits into from
Dec 13, 2024
Merged

sys/net/nanocoap: fix buffer overflow in separate response handling #21075

merged 5 commits into from
Dec 13, 2024

Conversation

maribu
Copy link
Member

@maribu maribu commented Dec 11, 2024

Contribution description

When RFC 8974 support (module nanocoap_token_ext) is in use, the request token may be longer than the buffer in the separate response context is large. This adds a check to not overflow the buffer.

Sadly, this is an API change: Preparing the separate response context can actually fail, so we need to report this with a return value.

The example application has been adapted to only proceed if the separate reply context could have been prepared, and rather directly emit a reset message if the token exceeds the static buffer.

Testing procedure

A second commit has been sneaked in to add the client_token shell command to tests/net/nanocoap_cli that allow specifying the Token as hex. The most recently set token is used in client.

Preparation

$  sudo ./dist/tools/tapsetup/tapsetup

Running the Server

This should be done with master and this PR

$ USEMODULE=nanocoap_token_ext make BOARD=native64 -C examples/nanocoap_server && examples/nanocoap_server/bin/native64/nanocoap_server.elf -w tap1
[...]
main(): This is RIOT! (Version: 2025.01-devel-261-g75828)
RIOT nanocoap example application
Waiting for address autoconfiguration...
{"IPv6 addresses": ["<IPV6_ADDR>"]}

copy the <IPV6_ADDR> part of the output

Running the Client

This should be done with this PR only (or cherry-picking the second commit onto master).

$ USEMODULE=nanocoap_token_ext make BOARD=native64 -C tests/net/nanocoap_cli flash term
[...]
2024-12-11 18:10:58,750 # RIOT native interrupts/signals initialized.
2024-12-11 18:10:58,751 # RIOT native board initialized.
2024-12-11 18:10:58,751 # RIOT native hardware initialization complete.
2024-12-11 18:10:58,751 # 
2024-12-11 18:10:58,752 # main(): This is RIOT! (Version: 2025.01-devel-262-g562036-sys/net/nanocoap/buffer-overflow-separate-response)
2024-12-11 18:10:58,752 # nanocoap test app
2024-12-11 18:10:58,752 # All up, running the shell now
> client_token deadbeefdeadbeefdeadbeef
2024-12-11 18:12:11,060 # client_token deadbeefdeadbeefdeadbeef
> client get fe80::24de:d0ff:fe85:bdb2 5683 /separate
2024-12-11 18:12:27,736 # client get fe80::24de:d0ff:fe85:bdb2 5683 /separate
2024-12-11 18:12:27,736 # nanocli: sending msg ID 1, 25 bytes
2024-12-11 18:13:45,417 # nanocli: msg send failed: -110

Output

On master, this will result in

RIOT native interrupts/signals initialized.
RIOT native board initialized.
RIOT native hardware initialization complete.

main(): This is RIOT! (Version: 2025.01-devel-261-g75828)
RIOT nanocoap example application
Waiting for address autoconfiguration...
{"IPv6 addresses": ["fe80::24de:d0ff:fe85:bdb2"]}
_separate_handler(): send ACK, schedule response
_separate_handler(): send delayed response
*** stack smashing detected ***: terminated
[1]    367561 IOT instruction (core dumped)  examples/nanocoap_server/bin/native64/nanocoap_server.elf -w tap1

With this PR, a RST message is replied and no buffer overflow / crash should appear.

Issues/PRs references

None

@maribu maribu added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Dec 11, 2024
@github-actions github-actions bot added Area: network Area: Networking Area: tests Area: tests and testing framework Area: CoAP Area: Constrained Application Protocol implementations Area: sys Area: System Area: examples Area: Example Applications labels Dec 11, 2024
@riot-ci
Copy link

riot-ci commented Dec 11, 2024 •
edited
Loading

Murdock results

✔️ PASSED

c7af4b2 sys/net/nanocoap: fix invalid RST messages

Success Failures Total Runtime
10249 0 10249 19m:26s

Artifacts

benpicco
benpicco reviewed Dec 11, 2024
View reviewed changes
sys/net/application_layer/nanocoap/sock.c Outdated Show resolved Hide resolved
sys/picolibc_syscalls_default/syscalls.c Outdated Show resolved Hide resolved
tests/net/nanocoap_cli/nanocli_client.c Outdated Show resolved Hide resolved
tests/net/nanocoap_cli/nanocli_client.c Outdated Show resolved Hide resolved
@maribu maribu force-pushed the sys/net/nanocoap/buffer-overflow-separate-response branch from 41d8af4 to 6221061 Compare December 11, 2024 19:20
@maribu maribu requested a review from kaspar030 as a code owner December 11, 2024 19:20
@maribu maribu force-pushed the sys/net/nanocoap/buffer-overflow-separate-response branch from 6221061 to 7e0c535 Compare December 11, 2024 19:50
@maribu maribu force-pushed the sys/net/nanocoap/buffer-overflow-separate-response branch from 7e0c535 to 4c46407 Compare December 11, 2024 19:55
@maribu maribu added the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Dec 11, 2024
@maribu maribu force-pushed the sys/net/nanocoap/buffer-overflow-separate-response branch from 4c46407 to c391765 Compare December 11, 2024 20:43
benpicco
benpicco approved these changes Dec 12, 2024
View reviewed changes
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even makes then code nicer :)

@maribu maribu added this pull request to the merge queue Dec 12, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 12, 2024
maribu and others added 5 commits December 12, 2024 14:28
@maribu
cpu/avr8_common: fix C++ compatibility of unistd.h
5fe7a2e 
C++ does not know about `restrict`, but both g++ and clang++ support
`__restrict`, as do `clang` and GCC [1].

Using `__restrict` instead of `restrict` is also what glibc does.

[1]: https://en.wikipedia.org/wiki/Restrict#Support_by_C++_compilers
@maribu @benpicco
sys/net/nanocoap: fix buffer overflow in separate response handling
7a738d0 
When RFC 8974 support (module `nanocoap_token_ext`) is in use, the
request token may be longer than the buffer in the separate response
context is large. This adds a check to not overflow the buffer.

Sadly, this is an API change: Preparing the separate response context
can actually fail, so we need to report this with a return value.

The example application has been adapted to only proceed if the separate
reply context could have been prepared, and rather directly emit a
reset message if the token exceeds the static buffer.

Co-authored-by: benpicco <[email protected]>
@maribu
sys/fmt: add scn_buf_hex()
6b12793 
This adds a function to convert a hex string to a byte array.
@maribu @benpicco
tests/net/nanocoap_cli: allow specifying the token
fa011a8 
This adds the client_token shell command that allows to specify the
CoAP Token. This is particularly useful to test extended length Tokens,
as enabled with module `nanocoap_token_ext`.

Co-authored-by: benpicco <[email protected]>
@maribu
sys/net/nanocoap: fix invalid RST messages
c7af4b2 
An RST message has no token, so don't reply with a token when sending
RST.

This also adds unit tests to ensure this this exact bug does not sneak
back in.
@maribu maribu force-pushed the sys/net/nanocoap/buffer-overflow-separate-response branch from c391765 to c7af4b2 Compare December 12, 2024 13:28
@github-actions github-actions bot added Platform: AVR Platform: This PR/issue effects AVR-based platforms Area: cpu Area: CPU/MCU ports labels Dec 12, 2024
@maribu maribu enabled auto-merge December 12, 2024 13:28
@maribu maribu added this pull request to the merge queue Dec 12, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 12, 2024
@maribu maribu added this pull request to the merge queue Dec 12, 2024
Merged via the queue into RIOT-OS:master with commit 28753e3 Dec 13, 2024
26 checks passed
@maribu
Copy link
Member Author

maribu commented Dec 13, 2024

Thx a lot!

@maribu maribu deleted the sys/net/nanocoap/buffer-overflow-separate-response branch December 13, 2024 07:21
@maribu maribu mentioned this pull request Dec 13, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nandojve nandojve nandojve approved these changes

@benpicco benpicco benpicco approved these changes

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

@miri64 miri64 Awaiting requested review from miri64 miri64 is a code owner

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann PeterKietzmann is a code owner

@kaspar030 kaspar030 Awaiting requested review from kaspar030 kaspar030 is a code owner

@kYc0o kYc0o Awaiting requested review from kYc0o kYc0o is a code owner

Assignees
No one assigned
Labels
Area: CoAP Area: Constrained Application Protocol implementations Area: cpu Area: CPU/MCU ports Area: examples Area: Example Applications Area: network Area: Networking Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: AVR Platform: This PR/issue effects AVR-based platforms Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@maribu @riot-ci @benpicco @nandojve